Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

A collection of small fixes #623

Merged
merged 9 commits into from
Mar 7, 2018
Merged

Conversation

austinbyers
Copy link
Contributor

@austinbyers austinbyers commented Mar 7, 2018

to: @chunyong-lin
cc: @airbnb/streamalert-maintainers
size: small|medium|large
resolves: #267
resolves: #572

Background

A collection of small fixes and features that I've accumulated

Changes

  • Add SNS and SQS outputs #620 added support for SNS and SQS outputs, but did not update the alert tests to mock those outputs. This has been fixed
  • Per Bug: terraform doesn't change DynamoDB correctly when AutoScaling enabled #572 , Dynamo autoscaling works best with the service role automatically generated by AWS. We use that instead of creating our own IAM role
  • Run terraform fmt to fix a spacing issue which snuck in
  • The third_party_libraries config option is generally only applicable to the rule processor, where users can specify additional libraries to install for their rules to work. To reduce confusion, this config option has been removed from the other default lambda configs (although it will still work for other functions if it is defined).
  • Adds a BinaryAlert YARA match rule (the log schema was added in Add BinaryAlert log schema #556, but no rules were added)
  • One of the unit tests was consuming 5-10% of the test time because it was not properly mocked:
[success] 10.41% tests.unit.stream_alert_athena_partition_refresh.test_main.TestStreamAlertAthenaGlobals.test_handler_no_unique_buckets: 0.9611s
from __future__ import absolute_import

I have no idea why it works, but I found this solution in the PyCharm blog

Testing

  • Deployed to a test account - alerts worked end-to-end
  • terraform validate
  • CI

@austinbyers austinbyers requested a review from chunyong-lin March 7, 2018 00:01
Copy link
Contributor

@chunyong-lin chunyong-lin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Asking for two more favors. Other then that LGTM.

resources = ["*"]
}
}

resource "aws_appautoscaling_target" "dynamodb_table_read_target" {
count = "${var.autoscale ? 1 : 0}"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you also help remove this line and autoscale variable? Because deleting custom static IAM role, it implies the autoscaling is enable all the time and can not be disabled.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is another line below should be deleted.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea, thanks!

@@ -13,6 +13,7 @@
See the License for the specific language governing permissions and
limitations under the License.
"""
from __future__ import absolute_import # Suppresses RuntimeWarning import error in Lambda
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Magic! 💥

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.03%) to 95.688% when pulling ec8d787 on austin-small-fixes into ebc2297 on release-2.0.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants